Preserve defaultValue literals #3074

leebyron · 2021-05-10T19:20:24Z

Depends on #3092
Fixes #3051

This change solves the problem of default values defined via SDL not always resolving correctly through introspection by preserving the original GraphQL literal in the schema definition. This changes argument and input field definitions defaultValue field from just the "value" to a new GraphQLDefaultValueUsage type which contains either or both "value" and "literal" fields.

Since either of these fields may be set, new functions for resolving a value or literal from either have been added - getLiteralDefaultValue and getCoercedDefaultValue - these replace uses that either assumed a set value or were manually converting a value back to a literal.

Here is the flow for how a default value defined in an SDL would be converted into a functional schema and back to an SDL:

Before this change:

(SDL) --parse-> (AST) --coerceInputLiteral--> (defaultValue config) --valueToAST--> (AST) --print --> (SDL)

coerceInputLiteral performs coercion which is a one-way function, and valueToAST is unsafe and set to be deprecated in #3049.

After this change:

(SDL) --parse-> (defaultValue literal config) --print --> (SDL)

andimarek · 2021-05-10T20:27:27Z

I think that is the right direction and as far as I understand you want that merged before the other bigger changes.

The more comprehensive end state we are aiming for In GraphQL Java is this:

public enum ValueState {
    NOT_SET,
    LITERAL,
    EXTERNAL_VALUE,
    INTERNAL_VALUE // this is deprecated and should not be used going forward, will be removed in the future
}

to capture the different states of the default value:
NOT_SET you probably don't need because you JS has undefined.

This is needed because we need to differentiate between external input value which will be converted to literal via valueToLiteral and the legacy behaviour of using a not well defined already coerced internal value, which will continue to use astFromValue.

leebyron · 2021-05-10T21:56:45Z

Yeah, I'm treating this as a prerequisite dependency of #3049, similar to a few of the other changes I have up right now. I'm hoping to keep that diff as focused as possible, and this change should have very little effect other than resolving this issue.

The more comprehensive end state we are aiming for In GraphQL Java is this

That sounds right to me! Yes we use undefined to represent not set. I'm also considering performing the uncoerce step at schema construction time so that "INTERNAL_VALUE" is only a state the configuration can be in, rather than the final reified schema types. But that's for the later change :)

src/type/defaultValues.js

leebyron · 2021-05-11T17:47:50Z

Updated to do proper memoization and error if multiple sources of truth are presented in a config

Depends on #3074 By way of introducing type `VariableValues`, allows `getVariableValues` to return both the coerced values as well as the original sources, which are then made available in `ExecutionContext`. While variable sources are not used directly here, they're used directly in #3065. This PR is pulled out as a pre-req to aid review

Fixes #3051 This change solves the problem of default values defined via SDL not always resolving correctly through introspection by preserving the original GraphQL literal in the schema definition. This changes argument and input field definitions `defaultValue` field from just the "value" to a new `GraphQLDefaultValueUsage` type which contains either or both "value" and "literal" fields. Since either of these fields may be set, new functions for resolving a value or literal from either have been added - `getLiteralDefaultValue` and `getCoercedDefaultValue` - these replace uses that either assumed a set value or were manually converting a value back to a literal. Here is the flow for how a default value defined in an SDL would be converted into a functional schema and back to an SDL: **Before this change:** ``` (SDL) --parse-> (AST) --coerceInputLiteral--> (defaultValue config) --valueToAST--> (AST) --print --> (SDL) ``` `coerceInputLiteral` performs coercion which is a one-way function, and `valueToAST` is unsafe and set to be deprecated in #3049. **After this change:** ``` (SDL) --parse-> (defaultValue literal config) --print --> (SDL) ```

@leebyron

[#3074 rebased on main](#3074). Depends on #3809 @leebyron comments from original PR (edited, hopefully correctly): > Fixes #3051 > > This change solves the problem of default values defined via SDL not always resolving correctly through introspection by preserving the original GraphQL literal in the schema definition. This changes argument and input field definitions `defaultValue` field from just the "value" to a new `GraphQLDefaultValueUsage` type which contains either (EDIT: but not both!) "value" and "literal" fields. > > Here is the flow for how a default value defined in an SDL would be converted into a functional schema and back to an SDL: > > **Before this change:** > > ``` > (SDL) --parse-> (AST) --coerceInputLiteral--> (defaultValue config) --valueToAST--> (AST) --print --> (SDL) > ``` > > `coerceInputLiteral` performs coercion which is a one-way function, and `valueToAST` is unsafe and set to be deprecated in #3049. > > **After this change:** > > ``` > (SDL) --parse-> (defaultValue literal config) --print --> (SDL) > ``` Co-authored-by: Lee Byron <[email protected]>

yaacovCR · 2024-09-23T13:00:48Z

Merged as #3810

@leebyron

[#3049 rebased on main](#3049). This is the last rebased PR from the original PR stack concluding with #3049. * Rebased: #3809 [Original: #3092] * Rebased: #3810 [Original: #3074] * Rebased: #3811 [Original: #3077] * Rebased: #3812 [Original: #3065] * Rebased: #3813 [Original: #3086] * Rebased: #3814 (this PR) [Original: #3049] Update: #3044 and #3145 have been separated from this stack. Changes from original PR: 1. `astFromValue()` is deprecated instead of being removed. @leebyron comments from #3049, the original PR: > Implements [graphql/graphql-spec#793](graphql/graphql-spec#793) > > * BREAKING: Changes default values from being represented as an assumed-coerced "internal input value" to a pre-coerced "external input value" (See chart below). > This allows programmatically provided default values to be represented in the same domain as values sent to the service via variable values, and allows it to have well defined methods for both transforming into a printed GraphQL literal string for introspection / schema printing (via `valueToLiteral()`) or coercing into an "internal input value" for use at runtime (via `coerceInputValue()`) > To support this change in value type, this PR adds two related behavioral changes: > > * Adds coercion of default values from external to internal at runtime (within `coerceInputValue()`) > * Removes `astFromValue()`, replacing it with `valueToLiteral()` for use in introspection and schema printing. `astFromValue()` performed unsafe "uncoercion" to convert an "Internal input value" directly to a "GraphQL Literal AST", where `valueToLiteral()` performs a well defined transform from "External input value" to "GraphQL Literal AST". > * Adds validation of default values during schema validation. > Since assumed-coerced "internal" values may not pass "external" validation (for example, Enum values), an additional validation error has been included which attempts to detect this case and report a strategy for resolution. > > Here's a broad overview of the intended end state: > > ![GraphQL Value Flow](https://user-images.githubusercontent.com/50130/118379946-51ac5300-b593-11eb-839f-c483ecfbc875.png) --------- Co-authored-by: Lee Byron <[email protected]>

leebyron requested review from andimarek and IvanGoncharov May 10, 2021 19:20

leebyron force-pushed the default-value-literals branch 2 times, most recently from 4b4cb69 to 3ab211e Compare May 10, 2021 21:53

leebyron force-pushed the input-value-config branch from c853294 to 1d87c02 Compare May 10, 2021 22:24

leebyron force-pushed the default-value-literals branch from 3ab211e to 222caaf Compare May 10, 2021 22:28

yaacovCR reviewed May 11, 2021

View reviewed changes

src/type/defaultValues.js Outdated Show resolved Hide resolved

leebyron force-pushed the default-value-literals branch from 222caaf to 0a91e63 Compare May 11, 2021 04:42

leebyron mentioned this pull request May 11, 2021

Add valueToLiteral() #3065

Closed

leebyron force-pushed the default-value-literals branch from 0a91e63 to 2ee1da8 Compare May 11, 2021 17:47

leebyron mentioned this pull request May 11, 2021

Preserve sources of variable values #3077

Closed

leebyron force-pushed the input-value-config branch from 1d87c02 to 9830fda Compare May 12, 2021 16:38

leebyron force-pushed the default-value-literals branch from 2ee1da8 to 330f01a Compare May 12, 2021 16:44

leebyron force-pushed the default-value-literals branch from 330f01a to 05db9bd Compare May 13, 2021 02:56

leebyron force-pushed the default-value-literals branch from 05db9bd to 6c79c53 Compare May 14, 2021 23:26

leebyron force-pushed the default-value-literals branch 2 times, most recently from 297f2c3 to 13e3148 Compare May 15, 2021 05:00

leebyron changed the base branch from input-value-config to coerce-input-literal May 15, 2021 05:01

leebyron force-pushed the default-value-literals branch from 13e3148 to 780cec2 Compare May 15, 2021 05:03

leebyron force-pushed the default-value-literals branch from 780cec2 to 5686d3e Compare May 15, 2021 06:24

leebyron force-pushed the coerce-input-literal branch from 5e2b6d5 to e4243d8 Compare May 15, 2021 06:50

leebyron force-pushed the default-value-literals branch from 5686d3e to 705f154 Compare May 15, 2021 06:51

leebyron force-pushed the coerce-input-literal branch from e4243d8 to 7dd6206 Compare May 15, 2021 21:31

leebyron force-pushed the default-value-literals branch from 705f154 to dd31191 Compare May 15, 2021 21:33

leebyron mentioned this pull request May 15, 2021

RFC: Default value validation & coercion #3049

Closed

jjsanchz46 approved these changes May 16, 2021

View reviewed changes

leebyron force-pushed the coerce-input-literal branch from 7dd6206 to 2f04eed Compare June 1, 2021 18:56

leebyron force-pushed the default-value-literals branch 2 times, most recently from 448367e to e2002dc Compare June 1, 2021 19:49

leebyron added the PR: bug fix 🐞 requires increase of "patch" version number label Jun 1, 2021

leebyron force-pushed the coerce-input-literal branch from 2f04eed to 15223bc Compare June 1, 2021 21:42

leebyron force-pushed the default-value-literals branch from e2002dc to 5601fd4 Compare June 1, 2021 21:42

leebyron force-pushed the coerce-input-literal branch from 15223bc to a79c0e5 Compare June 1, 2021 22:06

leebyron force-pushed the default-value-literals branch from 5601fd4 to 3110ca1 Compare June 1, 2021 22:07

leebyron force-pushed the coerce-input-literal branch from a79c0e5 to 8abb052 Compare June 3, 2021 17:08

leebyron force-pushed the default-value-literals branch from 3110ca1 to f1a14c6 Compare June 3, 2021 17:09

This was referenced Dec 31, 2022

Preserve defaultValue literals #3810

Merged

RFC: Default value validation & coercion #3814

Merged

yaacovCR closed this Sep 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preserve defaultValue literals #3074

Preserve defaultValue literals #3074

leebyron commented May 10, 2021 •

edited

Loading

andimarek commented May 10, 2021

leebyron commented May 10, 2021

leebyron commented May 11, 2021

yaacovCR commented Sep 23, 2024

Preserve defaultValue literals #3074

Preserve defaultValue literals #3074

Conversation

leebyron commented May 10, 2021 • edited Loading

andimarek commented May 10, 2021

leebyron commented May 10, 2021

leebyron commented May 11, 2021

yaacovCR commented Sep 23, 2024

leebyron commented May 10, 2021 •

edited

Loading